-
Notifications
You must be signed in to change notification settings - Fork 24
Bug/inspect subcommand #124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Warning Rate limit exceeded@agustingroh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThis update introduces version 1.25.2, updating the version string and changelog. It refactors component extraction in several modules to use a centralized helper for license list conversion. The policy check logic is enhanced to handle missing data more robustly and to prioritize licenses based on their source. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant PolicyCheck
User->>CLI: Run 'inspect copyleft' or 'inspect policy'
CLI->>PolicyCheck: Parse scanner results
PolicyCheck->>PolicyCheck: _get_components_data()
PolicyCheck->>PolicyCheck: _append_component() (with license source prioritization)
PolicyCheck->>PolicyCheck: _convert_components_to_list()
PolicyCheck-->>CLI: Return processed components with prioritized licenses
CLI-->>User: Output inspection results
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/scanoss/inspection/copyleft.py (1)
210-213
: SUCCESS / FAIL logic reversed
run()
returnsFAIL
when no copyleft components are found:if len(copyleft_components) <= 0: return PolicyStatus.FAIL.value, resultsTypically the check should succeed (SUCCESS) when no violations exist and fail only when copyleft components are present.
- if len(copyleft_components) <= 0: - return PolicyStatus.FAIL.value, results - return PolicyStatus.SUCCESS.value, results + if len(copyleft_components) > 0: + return PolicyStatus.FAIL.value, results + return PolicyStatus.SUCCESS.value, results
🧹 Nitpick comments (4)
CHANGELOG.md (1)
12-17
: Changelog entry OK – minor nitConsider adding a short “### Security” (even if empty) for consistency with older releases.
src/scanoss/inspection/copyleft.py (1)
64-68
: Avoid shadowing the built-informat
function
self.format = format
overwrites Python’s built-informat
.
Rename the attribute (e.g.format_type
) to prevent confusion and future bugs.src/scanoss/inspection/undeclared_component.py (1)
64-66
: Same built-in shadowing as in copyleft module
self.format = format
masks the built-informat
. Rename toformat_type
for clarity.src/scanoss/inspection/policy_check.py (1)
441-480
: License-priority helper – minor efficiency / style notes
- Converting
licenses_by_source
todict[str, dict]
then back to list duplicates memory; iterate once and return when first priority hit to save work.- Missing space after comma in the signature:
def _get_licenses_order_by_source_priority(self, licenses_data):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md
(2 hunks)src/scanoss/__init__.py
(1 hunks)src/scanoss/inspection/copyleft.py
(1 hunks)src/scanoss/inspection/policy_check.py
(4 hunks)src/scanoss/inspection/undeclared_component.py
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[duplication] ~13-~13: Possible typo: you repeated a word.
Context: ...hanges... ## [1.25.2] - 2025-06-18 ### Fixed - Fixed errors when no versions are declared in...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/scanoss/__init__.py (1)
25-25
: Version bump looks goodNothing else to flag here.
src/scanoss/inspection/undeclared_component.py (1)
257-257
: Good reuse of shared helperSwitching to
_convert_components_to_list
keeps logic consistent across inspectors.
476c8b1
to
9d18193
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_policy_inspect.py (1)
257-262
: Duplicate check – same fragility comment as aboveSee the previous note: relying on exact whitespace makes the test noisy for benign formatting tweaks. A small helper that normalises markdown rows would keep maintenance cost low.
🧹 Nitpick comments (4)
tests/test_policy_inspect.py (4)
117-118
: Hard-coding the new expected size may hide future regressionsThe update from
3
→2
components appears correct given the refactor, but the assertion is still a magic number.
Consider asserting against the length ofresults['summary'].split()
or explicitly checking the purls you expect to survive the new prioritisation – this future-proofs the test if another priority tweak changes the count again.
150-155
: Brittle comparison due to literal newline character
expected_summary_output
ends with a trailing\n
. On Windows or when refactoring the formatter this may flip to\r\n
, causing a false-negative.
Prefer stripping line endings before the equality check:-expected_summary_output = '2 component(s) with copyleft licenses were found.\n' +expected_summary_output = '2 component(s) with copyleft licenses were found.' ... -self.assertEqual(results['summary'], expected_summary_output) +self.assertEqual(results['summary'].strip(), expected_summary_output)
214-219
: String-equality on Markdown table rows is fragileThe test will fail if column order or spacing changes even though the semantic information is identical.
Instead of literal equality, parse the markdown (or simply split the line on|
) and assert on the tuple(purl, version, license)
for each row.
333-337
: Jira-MD expectations suffer from the same brittlenessLiteral pipe-separated strings are prone to break when the renderer changes alignment. Consider tokenising the line and asserting on the fields, or at least normalising whitespace.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_policy_inspect.py
(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
…clared on results
9d18193
to
1aab412
Compare
WHAT
Fixed
inspect
subcommandChanged
inspect copyleft
subcommandSummary by CodeRabbit
Bug Fixes
inspect
subcommand. Components without versions are now handled gracefully.New Features
inspect copyleft
subcommand now prioritizes licenses based on source priority, ensuring more consistent license filtering.Chores